Description

setDeployment now runs two merge2.MergeStrings passes whenever spec.deployment.patch is set,
doubling YAML parse/merge work for every reconcile. This is intentional as a workaround, but it
still adds ongoing CPU/latency overhead for patched deployments.

Code

pkg/model/deployment.go[R238-243]

+			merged, err := merge2.MergeStrings(string(conf.Raw), string(deplStr), false, kyaml.MergeOptions{ListIncreaseDirection: kyaml.MergeOptionsListPrepend})
+			if err != nil {
+				return fmt.Errorf("can not merge spec.deployment: %w", err)
+			}
+
+			merged, err = merge2.MergeStrings(string(conf.Raw), merged, false, kyaml.MergeOptions{})

Evidence

The PR introduces a second merge call in setDeployment; addToModel invokes setDeployment
during object initialization, which is part of the reconcile/model build path.

pkg/model/deployment.go[85-113]
pkg/model/deployment.go[223-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pkg/model/deployment.go` always performs a two-pass merge for `spec.deployment.patch`. This doubles YAML parsing/merging cost on every reconcile even when the patch doesn't need the `$patch` workaround.

### Issue Context
The first pass uses `ListPrepend` to get list ordering; the second pass re-applies the same patch with default options to support `$patch` directives.

### Fix Focus Areas
- pkg/model/deployment.go[223-246]

### Suggested approach
- Detect whether the patch actually contains `$patch` directives (e.g., a cheap substring check on `conf.Raw` for `"$patch"` / `$patch:`) and only run the second pass when needed.
- Optionally, also skip the first pass when the patch does not contain any list fields where ordering matters (if a safe/cheap detection is available), otherwise keep pass1.
- Keep behavior identical for patches that do contain `$patch`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

Both merge passes wrap errors with the same message, so logs don’t indicate whether pass 1
(ListPrepend) or pass 2 (default) failed without digging into the underlying error details. This
complicates debugging operational failures from malformed patches.

Code

pkg/model/deployment.go[R238-246]

+			merged, err := merge2.MergeStrings(string(conf.Raw), string(deplStr), false, kyaml.MergeOptions{ListIncreaseDirection: kyaml.MergeOptionsListPrepend})
+			if err != nil {
+				return fmt.Errorf("can not merge spec.deployment: %w", err)
+			}
+
+			merged, err = merge2.MergeStrings(string(conf.Raw), merged, false, kyaml.MergeOptions{})
			if err != nil {
				return fmt.Errorf("can not merge spec.deployment: %w", err)
			}

Evidence

Both merge calls return the same wrapped error string, even though they run with different merge
options and have different failure modes.

pkg/model/deployment.go[238-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Two different merge operations can fail, but both return `fmt.Errorf("can not merge spec.deployment: %w", err)`.

### Issue Context
With the two-pass approach, knowing which pass failed is important for quickly identifying whether the failure is tied to the prepend strategy or to `$patch` directive handling.

### Fix Focus Areas
- pkg/model/deployment.go[238-246]

### Suggested approach
- Change the wrapped error messages to include the pass number and merge mode, e.g.:
 - `can not merge spec.deployment (pass 1: ListPrepend): %w`
 - `can not merge spec.deployment (pass 2: default): %w`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

TestInitContainerOrderInSpecDeployment asserts the initContainer list length equals the expected
list, so adding any new default initContainer to the base manifest will fail these cases even if the
relative ordering guarantee still holds. This makes the test unnecessarily sensitive to unrelated
default-manifest evolution.

Code

pkg/model/deployment_test.go[R242-246]

+			initContainers := model.backstageDeployment.podSpec().InitContainers
+			assert.Equal(t, len(tt.expected), len(initContainers))
+			for i, name := range tt.expected {
+				assert.Equal(t, name, initContainers[i].Name)
+			}

Evidence

The test currently enforces len(initContainers) == len(expected) before checking name-by-index,
which ties the test to the exact default initContainer count.

pkg/model/deployment_test.go[161-249]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The initContainer ordering test asserts exact slice length, which will break whenever the default deployment gains additional init containers.

### Issue Context
The intent is to validate relative ordering around `install-dynamic-plugins`, not to freeze the entire initContainer inventory.

### Fix Focus Areas
- pkg/model/deployment_test.go[242-246]

### Suggested approach
- Replace the strict length assertion with checks that:
 - required containers exist (e.g., find indices by name), and
 - their indices satisfy the expected ordering (e.g., `idx("my-init") < idx("install-dynamic-plugins")`).
- Keep a focused assertion that `install-dynamic-plugins` remains present when expected.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

• Implement two-pass merge strategy to honor init container ordering in spec.deployment.patch
  - Pass 1 uses ListPrepend to prepend new list items before existing ones
  - Pass 2 applies $patch directives to preserve special merge semantics
• Add comprehensive test coverage for init container ordering scenarios
  - Tests prepending before existing containers
  - Tests anchoring to place containers after existing ones
  - Tests multiple containers with mixed ordering
• Document list ordering semantics and provide usage examples
  - Explain prepend behavior for new list items
  - Show workaround to anchor containers after existing ones by name reference

Two-pass merge for init container ordering

• Implement two-pass merge strategy for spec.deployment.patch
• First pass uses ListIncreaseDirection: MergeOptionsListPrepend to prepend new list items
• Second pass uses default merge options to apply $patch directives
• Added TODO comment referencing kyaml issue #6146 for future cleanup

pkg/model/deployment.go

Add init container ordering tests

• Update existing TestMergeFromSpecDeployment assertions to reflect prepend behavior
• Add new TestInitContainerOrderInSpecDeployment with four test cases
• Test cases cover prepending before existing containers, anchoring by name, and mixed ordering
 scenarios
• Verify correct init container order in resulting deployment

pkg/model/deployment_test.go

Document list ordering semantics and workarounds

• Add new "List ordering" section explaining prepend behavior for new list items
• Document that items matching by name are merged in-place and retain position
• Provide example of prepending custom init container before install-dynamic-plugins
• Show workaround to anchor containers after existing ones by referencing them by name first

docs/configuration.md

In response to this:

/cherry-pick release-1.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.